x64: lower isub by a constant to lea - #14015

Pushed a fixup: refreshed the 12 load-store/x64 disas goldens that the CLI disas test flagged. Those are dynamic-memory bounds checks that compute bound - access_size in place; with this rule that sub $C now goes through lea and degenerates to add $-C — the same instruction count, and identical to how iadd by a constant already lowers today (verified isub v, C and iadd v, -C produce byte-identical output). The win over the old mov+sub shows up when the result lands in a different register than the input.

Subscribe to Label Action

cc @cfallin, @fitzgen

Hi @darmie -- are you aware of the add-vs-lea performance discussion thread we had recently (#13325)? I ask because at the very least, we should benchmark this change with the full Sightglass suite. I am also pretty leery in general of introducing more uses of LEA given the performance variability that we've seen on different systems. I'm not surprised that you saw a speedup on one particular benchmark since LEA is "non-destructive" (doesn't clobber the source) but I'd be curious to know how this looks overall. Thanks!

Hi @cfallin! Actually I am not aware of that thread, I should have searched first. Yes I saw a performance improvement on my alderlake x86_64 box, but it's only ~6.9% so far. I'll read through the thread now.

@cfallin I have gone through the thread. From what I gather, lea performance is inconsistent on Xeon processors. Naive question: Is it possible to gate the implementation and skip Xeon machines for now?

We discussed this in the other thread a bit but we don't have per-microarchitecture machine models, and that is what it'd take (which is a really big project in the compiler). We don't want to have a patchwork of heuristics for things like this.

Can you clarify

Yes I saw a performance improvement on my alderlake x86_64 box, but it's only ~6.9% so far

do you mean on all of Sightglass, or on one particular benchmark?

do you mean on all of Sightglass, or on one particular benchmark?

It's a microbench I did for my project. I simply did multiple A/B runs – that was how I arrived at that score.

OK, I think we'd want Sightglass runs across multiple microarchitectures that show clear benefit here before deciding to take it for sure.

OK, I think we'd want Sightglass runs across multiple microarchitectures that show clear benefit here before deciding to take it for sure.

Yes I can do fhat. Is the Sightglass tool setup in the CI?

It's not in CI -- see the README at https://github.com/bytecodealliance/sightglass/ for more.

It's not in CI -- see the README at https://github.com/bytecodealliance/sightglass/ for more.

Oh.. thanks!

Sightglass results. The short version: this rule is a wash and I think it should be closed, but the run turned up something more interesting than the PR, so I want to put both in front of you.

Setup: wasmtime 3ebfbe5, sightglass 08180eb, all.suite (130 benchmarks), engines built from one tree differing only in the three codegen files. Ubuntu 26.04, i5-1250P (Alder Lake, family 6 model 154), pinned to the four P-cores. I couldn't disable turbo or set the governor on this box, so everything significant was re-run at 5x the samples with the engine order reversed, so drift would flip the sign instead of surviving.

This PR: no aggregate movement. Sum total across the suite is "no difference" for execution and compilation. Per benchmark, reproducing across both passes: quicksort 1.02 to 1.13x, spidermonkey-regex 1.03 to 1.05x, tinygo-json 1.02 to 1.07x and shootout-ed25519 ~1.01x faster with the rule; sqlite3, libsodium-pwhash_scrypt and shootout-sieve 1.01 to 1.02x slower. (libsodium-randombytes and shootout-memmove were significant in the first pass only, so I read those as noise.)

The rule fires heavily, so this is not a case of the suite failing to exercise it. AOT-compiling for x86_64 and counting mnemonics, spidermonkey-regex moves −13598 subl / +13215 leal / −14042 movq, and total instructions retired drop on every benchmark, including all three that got slower. sqlite3 executes 2530 fewer instructions and loses 1 to 2%.

The more useful result. To find out whether that was LEA being weak here or just this rule being marginal, I ran the counterfactual from #13325 on the same rig: main, with iadd_base_case_32_or_64_lea replaced by x64_add.

The wins are significant: libsodium-misuse 1.30 to 1.77x, shootout-nestedloop 1.12 to 1.62x, gcc-loops 1.19x, shootout-seqhash 1.14x, meshoptimizer 1.06 to 1.11x, blake3-scalar 1.06 to 1.10x. So on this core LEA-for-add is very strongly load-bearing, and the blunt "use ADD for adds always" option would be expensive here. That's one microarchitecture, and the opposite of what @bongjunj measured on Cascade Lake, so I offer it as a data point for that thread rather than a conclusion.

That also explains why my rule is a wash while the iadd one is not, and it's a difference in kind rather than degree. to_amode_add reaches amode_imm_reg_reg_shift, so the iadd rule collapses base + index*scale + disp, plus chained iadd-by-constant, into a single LEA. My rule only ever produces amode_imm_reg, i.e. lea -C(%base): it folds nothing and buys exactly one saved mov. It captures the weakest form of the LEA benefit while paying the full microarchitectural risk, which is the wrong end of the trade.

Given that, I don't think there's a case for this change and I'm happy to close it. If it's useful I can push the no-lea-for-add data to #13325 as a separate note, and if you'd like any of this re-run on the project's benchmarking hardware, a /bench_x64 here would do it.

Given that, I don't think there's a case for this change and I'm happy to close it. If it's useful I can push the no-lea-for-add data to #13325 as a separate note, and if you'd like any of this re-run on the project's benchmarking hardware, a /bench_x64 here would do it.

Hi @darmie -- this response looks like it is a direct copy/paste out of an AI session, complete with a suggestion to run a slash-command. Aside from not making sense in context (it makes no sense to tell me I can run a slash-command here in a GitHub thread -- it only made sense in your agent chat), this is a direct violation of our AI tool policy, which states that you cannot use an LLM's output directly in correspondence with other humans. You yourself may use such tools, but must review the output yourself, and correspond as a human with other humans. While we appreciate contributions, we want to mentor and collaborate with people, not indirectly with their AI bots through a lossy channel. Please make sure you adhere to this policy in any future contributions.

Given the technical data, as well as this policy violation issue (and the implication that you have not deeply understood the issue yourself but are directly copy/pasting from a bot in the thread), I will go ahead and close this PR. Thanks.

Given that, I don't think there's a case for this change and I'm happy to close it. If it's useful I can push the no-lea-for-add data to #13325 as a separate note, and if you'd like any of this re-run on the project's benchmarking hardware, a /bench_x64 here would do it.

Hi @darmie -- this response looks like it is a direct copy/paste out of an AI session, complete with a suggestion to run a slash-command. Aside from not making sense in context (it makes no sense to tell me I can run a slash-command here in a GitHub thread -- it only made sense in your agent chat), this is a direct violation of our AI tool policy, which states that you cannot use an LLM's output directly in correspondence with other humans. You yourself may use such tools, but must review the output yourself, and correspond as a human with other humans. While we appreciate contributions, we want to mentor and collaborate with people, not indirectly with their AI bots through a lossy channel. Please make sure you adhere to this policy in any future contributions.

Given the technical data, as well as this policy violation issue (and the implication that you have not deeply understood the issue yourself but are directly copy/pasting from a bot in the thread), I will go ahead and close this PR. Thanks.

@cfallin I apologize that it still reads that way even though it has been reviewed and edited by me. Yes I did use AI to gather my thoughts, but between the day you asked me to use Sightglass tool and now, I had run the benchmarks many times partly in hopes to justify this PR, but to give a clear verdict on where it stands. I am fully aware of the policy and I did honestly considered that while I also tried not to remove relevant information that you may find useful.